src/Testcontainers.Grafana/GrafanaContainer.cs (1)

32-42: Reconsider the need for this method given authentication patterns.

The GetConnectionString() method embeds credentials in the URI, but this approach has several concerns:

1. Security: Embedding credentials in URIs is problematic as they may be logged, cached, or exposed in stack traces.
2. Grafana authentication: Modern Grafana typically uses API keys or Bearer tokens in HTTP headers, not HTTP Basic Auth via URI.
3. Unused pattern: As noted in the previous review, tests authenticate using HTTP headers, suggesting this method may not align with actual usage patterns.

If this method isn't required by consumers or tests, consider removing it. Otherwise, clarify its intended use case.

If the method must remain, consider documenting its purpose and warning about security implications:

     /// <summary>
     /// Gets the Grafana connection string.
     /// </summary>
+    /// <remarks>
+    /// This method embeds credentials in the URI for backward compatibility.
+    /// For production use, prefer API keys with the Authorization header.
+    /// Avoid logging or caching the returned connection string as it contains credentials.
+    /// </remarks>
     /// <returns>The Grafana connection string.</returns>
     public string GetConnectionString()

Alternatively, if tests and typical usage rely on HTTP header authentication, remove this method:

-    /// <summary>
-    /// Gets the Grafana connection string.
-    /// </summary>
-    /// <returns>The Grafana connection string.</returns>
-    public string GetConnectionString()
-    {
-        var endpoint = GetHttpEndpoint();
-        var username = _configuration.Username ?? GrafanaBuilder.DefaultUsername;
-        var password = _configuration.Password ?? GrafanaBuilder.DefaultPassword;
-        return new UriBuilder(endpoint)
-        {
-            UserName = username,
-            Password = password
-        }.ToString();
-    }

src/Testcontainers.Grafana/GrafanaBuilder.cs (2)

83-116: Replace WithBindMount with WithResourceMapping for better compatibility.

As noted in previous reviews, mounting shares with WithBindMount can cause incompatibility with remote container runtimes. The recommended approach is to use WithResourceMapping(FileInfo, FileInfo) to copy files into the container.

Additionally, these provisioning methods lack test coverage. Consider adding tests or documenting the expected usage patterns.

Based on past review feedback.

---

133-133: Use WithAnonymousAccessDisabled() for consistency.

As noted in previous reviews, line 133 should use the WithAnonymousAccessDisabled() method instead of directly setting the environment variable.

Apply this diff:

             .WithUsername(DefaultUsername)
             .WithPassword(DefaultPassword)
-            .WithEnvironment("GF_AUTH_ANONYMOUS_ENABLED", "false")
+            .WithAnonymousAccessDisabled()
             .WithEnvironment("GF_AUTH_BASIC_ENABLED", "true")

Based on past review feedback.

tests/Testcontainers.Grafana.Tests/Testcontainers.Grafana.Tests.csproj (1)

3-3: Consider multi-targeting for broader test coverage.

The test project targets only net9.0, whereas the main Grafana project supports net8.0;net9.0;netstandard2.0;netstandard2.1. While this may be intentional for simplicity, testing against multiple frameworks ensures compatibility across the supported target frameworks.

If broader coverage is desired, apply this diff:

-    <TargetFrameworks>net9.0</TargetFrameworks>
+    <TargetFrameworks>net8.0;net9.0</TargetFrameworks>

tests/Testcontainers.Grafana.Tests/GrafanaContainerTest.cs (5)

27-28: Consider removing ConfigureAwait(true).

In test code, ConfigureAwait(true) is redundant since that's the default behavior. Omitting it simplifies the code without changing functionality.

Apply this diff to simplify:

-        using var httpResponse = await httpClient.GetAsync("api/health", TestContext.Current.CancellationToken)
-            .ConfigureAwait(true);
+        using var httpResponse = await httpClient.GetAsync("api/health", TestContext.Current.CancellationToken);

---

46-47: Consider removing ConfigureAwait(true).

Same as the previous test, ConfigureAwait(true) is redundant in test code.

Apply this diff:

-        using var httpResponse = await httpClient.GetAsync("api/org", TestContext.Current.CancellationToken)
-            .ConfigureAwait(true);
+        using var httpResponse = await httpClient.GetAsync("api/org", TestContext.Current.CancellationToken);

---

53-65: Consider validating the URI format.

While the current assertions verify that key components are present in the connection string, they don't validate that the connection string is a well-formed URI with credentials properly encoded.

Consider adding a URI parsing assertion:

     public void GetConnectionStringReturnsExpectedFormat()
     {
         // When
         var connectionString = _grafanaContainer.GetConnectionString();

         // Then
         Assert.NotNull(connectionString);
+        Assert.True(Uri.TryCreate(connectionString, UriKind.Absolute, out var uri), "Connection string should be a valid URI");
         Assert.Contains(GrafanaBuilder.DefaultUsername, connectionString);
         Assert.Contains(GrafanaBuilder.DefaultPassword, connectionString);
         Assert.Contains(_grafanaContainer.Hostname, connectionString);
     }

---

97-98: Consider removing ConfigureAwait(true).

Consistent with previous suggestions, ConfigureAwait(true) is redundant in test code.

Apply this diff:

-            using var httpResponse = await httpClient.GetAsync("api/org", TestContext.Current.CancellationToken)
-                .ConfigureAwait(true);
+            using var httpResponse = await httpClient.GetAsync("api/org", TestContext.Current.CancellationToken);

---

131-132: Consider removing ConfigureAwait(true).

Consistent with previous suggestions, ConfigureAwait(true) is redundant in test code.

Apply this diff:

-            using var httpResponse = await httpClient.GetAsync("api/org", TestContext.Current.CancellationToken)
-                .ConfigureAwait(true);
+            using var httpResponse = await httpClient.GetAsync("api/org", TestContext.Current.CancellationToken);

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro